cc @andrykonchin @eregon

The TruffleRuby build is failing because it's referencing the old name for the field. Would it be alright to merge this and have you fix it after?

Going forward, I wonder what we can do to reduce this kind of friction. These kinds of breaking changes of course wouldn't happen outside of a release, but I wouldn't want the TruffleRuby build to be broken for that long.

@kddnewton The usual strategy for this is to wait that TruffleRuby adopts the change, and then when that's merged we can merge here and no CI breaks.
So if this is not urgent I would wait for that.

@andrykonchin Could you make a small PR to adopt that change?

Okay, let me know when it's ready.

I'm closing out for the weekend so going to merge this. I really want to figure this out going forward, otherwise it's impossible to make breaking changes without waiting for y'all to make changes. Maybe we could mark the check as optional.

@kddnewton The usual strategy for this is to wait that TruffleRuby adopts the change, and then when that's merged we can merge here and no CI breaks. So if this is not urgent I would wait for that.

@eregon @kddnewton I can see the benefits of wanting people to see breaking changes on TR for bug fixes, but for development work this creates a backwards dependency.

If I submit a PR here (X) to change something then if we followed your process and I was willing to submit a PR (Y) to your repo first (and let's just pretend that is no work but just something which just happens); then I think this assumes YARP development is some single linear process.

So Y is landed, then someone in yarp fixes a different bug with a PR (Z), then we land X. In this case Z is red and has nothing to do with the PR you fixed with Y. That will be confusing to the author of Z. Other scenarios (WIP PRs, language level changes needing githubaction changes depending on language level TR has) but right now this one feels like it would be common.

If we adopted the same process for JRuby, then we have to have two consuming repositories do PRs before the PR can land on YARP. That ignores some of YARPs CI will catch things which would be hard to test locally (so it would be nice to see that before two PRs land in other projects).

I wonder if there is some other way of achieving the goals here. Correct me if I am wrong @eregon but you want to make sure if someone breaks TR from a fix there is a chance for them to notice that and reject the PR (or at least consult about what happened).

I think the main issue is once it's red nobody will notice if there is another incompatible change or a bug slipping in.
And so it might be hard to get back to green if that happens (e.g. if Java deserialization is broken and has multiple bugs it quickly becomes challenging to fix it, that already happened before the check was there).

I think the existing process of merging in TruffleRuby first and in YARP right after works well for most cases.

The main alternative I see is to avoid breaking that check, for instance in this case by keeping the old field or so. It often feels hacky or overhead though or it might not be reasonably possible.

I also thought at some point to only do the "can it deserialize cleanly" check (maybe running it on JRuby if it doesn't depend on specific YARP node names and field names, or even running Loader on plain java). But that's weaker in terms of testing than actually checking it works for a real usage of YARP, i.e., as TruffleRuby uses it. I think so far this usage is the only one that really checks the AST makes sense and has the relevant information. Nothing else in CI really uses YARP nodes for execution yet, e.g., if there was an incorrect node as the receiver of a CallNode, I think nothing would notice it except that check.

If we do a bunch of field/node renames then it probably makes sense to group them, and similarly for other related breaking changes, so there is less often the need to merge things in a particular order.